Skip to content

fix(gateway): register the shared ACME account under the rotation lock - #1138

Merged
kvinwang merged 5 commits into
nextfrom
fix/gateway-acme-register-race
Aug 26, 2026
Merged

fix(gateway): register the shared ACME account under the rotation lock#1138
kvinwang merged 5 commits into
nextfrom
fix/gateway-acme-register-race

Conversation

@kvinwang

@kvinwang kvinwang commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Problem

A cluster registers its shared ACME account lazily, on whichever renewal first
finds the credentials record empty:

let stored_creds = self.kv_store.get_acme_credentials()?;
if let Some(creds) = stored_creds { /* ... load and return ... */ }

// Create new global ACME account
let client = AcmeClient::new_account(acme_url, dns01_client, ...).await?;
self.kv_store.save_acme_credentials(&CertCredentials { acme_credentials: creds_json.clone() })?;
if let Some(account_uri) = extract_account_uri(&creds_json) {
    self.generate_and_save_acme_attestation(&account_uri).await?;
}

Nothing serializes that. The only lock held here is try_acquire_cert_lock,
which is per domain, so it does not order two domains against each other at
all:

  • one node, two domains — the periodic renewal task and an admin
    RenewZtDomainCert run on separate tasks and take separate locks
  • several nodes — each takes the lock for a different domain and proceeds

And the empty-record state is exactly what a fresh cluster starts in: every
node runs init_all at boot, so the first issuance of each domain races the
others.

Both registrations succeed at the CA. save_acme_credentials is a WaveKV write,
last-writer-wins with no compare-and-swap (try_acquire_rotation_lock's own
docs say so), so the record keeps one account and the other is lost. The cost:

  1. Every loser spends a registration the CA rate-limits — Let's Encrypt allows
    10 new accounts per IP per 3 hours.
  2. generate_and_save_acme_attestation writes under its own key and races
    separately, so the surviving attestation can be the other account's. The
    cluster then issues with an account it cannot prove it holds, which is
    visible to anyone verifying the attestation and not visible locally at all.
  3. Issuance itself converges — the next renewal reads the winner — but a
    SetCaa run in between pins CAA to the loser, and issuance stays broken
    until SetCaa is rerun.

Fix

One lock in the KV store now covers every operation over the shared account --
rotation, CAA reconciliation, and first-use registration -- because they all
read or re-pin the same thing:

let rotation_lock = self.acquire_acme_lock("register the shared ACME account")?;
let client = self.register_or_adopt_account(domain, &dns_cred, acme_url).await;

The re-read under it is the point: a waiter let through after the holder
finishes adopts the account that appeared rather than registering a second one.

That lock replaces caa_lock, the in-process mutex set_caa_all and rotation
shared. Two consequences.

Registration must not be reached from inside the locked region. The lock is
not reentrant -- a stale holder has to expire rather than be re-entered -- and
set_caa_all registers lazily from inside its per-domain loop on a fresh
cluster. So set_caa_all now registers the account before it takes the lock,
where the steady state costs one KV read and no provider or CA round trip. (An
earlier revision of this PR took caa_lock in the registration path instead,
which self-deadlocked on exactly this path: set_caa_all holds it across the
loop, tokio::sync::Mutex is not reentrant, and lock().await has no timeout,
so the first SetCaa on a fresh cluster hung the task and left the lock held
for the life of the process.)

Reconciliation is now ordered across nodes, not just within one. CAA
reconciliation only ever held the in-process mutex, so one node's SetCaa could
interleave with another's rotation over the same zone. Reconciling rewrites a
zone's issuer records in place -- guard, sweep, write, unguard -- and two runs
over one zone delete each other's records, which can leave the ; guards behind
and block issuance until a later run succeeds.

The DNS provider client is built after the lock is granted rather than in the
shared prologue. Constructing it resolves the zone through an authenticated
provider API call, and a run that is about to be refused should not spend one.
The fast path -- credentials already present -- is unchanged, and still builds
it.

The two paths that load a stored account (the fast path and the adopt path) now
share load_stored_acme_client, so the corrupt-record and wrong-directory
checks exist once.

Tests

  • set_caa_all_registers_the_account_before_taking_the_lock -- a fresh
    cluster's SetCaa completes (under a timeout, so a reentrant lock regression
    fails rather than hangs) and leaves the lock free
  • set_caa_all_rejects_concurrent_runs -- reconciliation is refused while any
    node holds the shared lock
  • first_use_registration_waits_for_the_rotation_lock -- registration is
    refused while the lock is held; the DNS client is built only after it is
    granted, so an unreachable provider cannot be what the run fails on
  • registration_rereads_the_record_under_the_lock -- the account that appeared
    while this call waited decides the outcome, without spending a registration
  • rotate_acme_credentials_rejects_concurrent_runs -- two in-process rotations
    are ordered by the same KV lock that orders two nodes

Copilot AI lite review requested due to automatic review settings August 25, 2026 15:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@kvinwang
kvinwang force-pushed the fix/gateway-acme-register-race branch 2 times, most recently from 41dc14e to 3893867 Compare August 26, 2026 04:32
@kvinwang
kvinwang enabled auto-merge (squash) August 26, 2026 04:40
@kvinwang
kvinwang disabled auto-merge August 26, 2026 04:40
@kvinwang
kvinwang enabled auto-merge August 26, 2026 04:40
Rotation, CAA reconciliation, and first-use registration all read or
re-pin the same account, so they now take the same KV lock. Two
consequences.

Registration under the lock cannot be reached from inside a region that
already holds it: set_caa_all held the in-process caa_lock across its
per-domain loop, and a fresh cluster's first SetCaa registers from inside
that loop -- which, with a non-reentrant tokio Mutex and no timeout, hung
the task and left the lock held for the life of the process. set_caa_all
now registers the account before it takes the lock, where the steady
state costs one KV read.

Dropping caa_lock for the KV lock also widens what is ordered: CAA
reconciliation was serialized within a process only, so one node's SetCaa
could interleave with another's rotation over the same zone. Reconciling
rewrites a zone's issuer records in place -- guard, sweep, write, unguard
-- and two runs over one zone delete each other's records, which can
leave the ";" guards behind and block issuance until a later run
succeeds.
Nothing under the ACME lock may take it again. `set_caa` now loads the
stored account instead of registering one, so the locked region cannot
reach `acquire_acme_lock` at all -- the caller registers up front, and
the property holds by construction rather than by argument.

The E2E suite never called SetCaa or RotateAcmeCredentials, so the two
admin operations that take the cluster-wide lock ran nowhere outside
single-process unit tests. Both nodes now reconcile CAA, then one rotates
the shared account and the other issues with it, which is also what
proves the switch reached the cluster rather than one process. The zone
is read back from the mock provider after each step: exactly one `issue`
and one `issuewild` pinned to the same account, no ";" guard left behind
-- the state a reconciliation that was interleaved, or that died halfway,
does not produce.

Admin RPCs are bounded by `--max-time` so an RPC that blocks forever
fails the suite where the cause is obvious, instead of hanging it until
the job timeout.
The suite was one indivisible run: KMS and Gateway 0.5.8 rolled to
current, with two apps. A change confined to one area had to pay for all
of it, and one area it could never reach at all -- a fresh cluster's
first ACME account registration, because 0.5.8 registers the shared
account long before the current binary starts.

A phase is now the unit a run can be limited to (`./run-e2e.sh --phase
certbot`, or DSTACK_E2E_PHASE). Each is self-contained: it deploys what
it needs and asserts on it. The `certbot` phase brings up a current KMS
and two current Gateway nodes, then reconciles CAA on a cluster holding
no account -- registering one from inside the region that holds the
cluster-wide ACME lock -- issues against it, requires the second node to
adopt that account rather than register its own, and rotates.

Only the upgrade phase boots the v0.5.11 compatibility image, so no other
phase requires it to be present, and none widens the authorization
allowlist with an OS digest it will never launch. `run-upgrade-e2e.sh`
stays as a wrapper.
Two things a current-only cluster hits that an upgraded one never does.

`max_dns_wait: 0` is rejected by current code at credential creation --
an issuance that never waits for propagation cannot succeed against a
real provider -- and only 0.5.8 ever accepted it. The upgrade phase
configures the cluster while the nodes still run 0.5.8, so the value
survives into current code as stored state and the validation is never
reached. One second is as good as none against a Pebble configured to
validate unconditionally.

Adding a ZT domain starts an issuance, and on a fresh cluster that
issuance registers the shared ACME account -- so an operator's SetCaa,
issued right after, races it for the cluster-wide lock and is refused.
That refusal is deliberate and says to retry after the holder finishes,
which is seconds for a registration. So retry, the way this suite
already retries the per-domain certificate lock. Any other error still
fails immediately.
@kvinwang
kvinwang force-pushed the fix/gateway-acme-register-race branch from 3893867 to e7532be Compare August 26, 2026 04:54
@kvinwang
kvinwang merged commit 027c62b into next Aug 26, 2026
19 checks passed
@kvinwang
kvinwang deleted the fix/gateway-acme-register-race branch August 26, 2026 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants